Skip to content

Ensure pool timeout is applied, even after attempts leading to ConnectionNotAvailable#823

Merged
karpetrosyan merged 5 commits intoencode:masterfrom
valsteen:fix-pooltimeout
Oct 12, 2023
Merged

Ensure pool timeout is applied, even after attempts leading to ConnectionNotAvailable#823
karpetrosyan merged 5 commits intoencode:masterfrom
valsteen:fix-pooltimeout

Conversation

@valsteen
Copy link
Contributor

@valsteen valsteen commented Oct 11, 2023 •
edited
Loading

Summary

fixes #822

This ensures the total time spent trying to acquire a connection is considered.

  • both sync and async versions are fixed
  • the first commit add failing tests contains a failing test case demonstrating the issue. The second commit fixes it
  • as before, a pool timeout of 0 will succeed if a connection is immediately available

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly – the documentation does not need change

@valsteen valsteen force-pushed the fix-pooltimeout branch 2 times, most recently from ac8bd4c to ee12daa Compare October 11, 2023 06:32
@valsteen valsteen marked this pull request as draft October 11, 2023 06:40
@valsteen valsteen force-pushed the fix-pooltimeout branch 3 times, most recently from 0c49e79 to 10742de Compare October 11, 2023 08:21
@karpetrosyan
Copy link
Contributor

karpetrosyan commented Oct 11, 2023

I believe that changes such as "quote to double quote" are unnecessary in this pull request.
We also want to keep things as simple as possible, so the time module should be removed along with the async_open_nursery function.

The implementation makes sense, but the test cases are overly complicated; we could probably write them much more simply.

@valsteen valsteen force-pushed the fix-pooltimeout branch 2 times, most recently from fc79e17 to 9fa759d Compare October 11, 2023 08:29
@valsteen valsteen force-pushed the fix-pooltimeout branch 3 times, most recently from ce92bcf to cb112af Compare October 11, 2023 09:16
@valsteen valsteen marked this pull request as ready for review October 11, 2023 09:21
@karpetrosyan
Copy link
Contributor

karpetrosyan commented Oct 11, 2023

Let's just use the previous test case.
This test case is complicated enough to be ignored.
Also, I am not sure if these kinds of milisecond timeouts are reliable.

I have tried to write a simple test for these cases, but it seems like we should avoid that.
The implementation makes sense to me; let's just add a changelog, and I think it should be ready.

@valsteen
Copy link
Contributor Author

valsteen commented Oct 11, 2023

@karpetrosyan I understand, let me check if it can be isolated/simplified

@valsteen
Copy link
Contributor Author

valsteen commented Oct 12, 2023

tests reverted and Changelog added

@karpetrosyan
Copy link
Contributor

karpetrosyan commented Oct 12, 2023

This is great. Thank you @valsteen!

@karpetrosyan karpetrosyan merged commit f8ff1c4 into encode:master Oct 12, 2023
karpetrosyan pushed a commit to karpetrosyan/httpcore that referenced this pull request Oct 19, 2023
@valsteen @karpetrosyan
Ensure pool timeout is applied, even after attempts leading to Connec…
25e4cac 
…tionNotAvailable (encode#823)

* add failing tests

* fix pooltimeout

* Revert "add failing tests"

This reverts commit cacc248.

* mark `if timeout < 0` as not covered, since the test is reverted

* Update CHANGELOG.md
This was referenced Nov 2, 2023
Merged
Merged
@zanieb zanieb mentioned this pull request Nov 10, 2023
Closed
@sacOO7 sacOO7 mentioned this pull request Dec 10, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@karpetrosyan karpetrosyan karpetrosyan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Pool timeout doesn't work

2 participants

@valsteen @karpetrosyan